SOLR-8776: Support RankQuery in grouping #162
Conversation
diegoceccarelli
force-pushed
the
master-solr-8776
branch
from
cd33172 to
f3cc241
Compare
| @@ -39,6 +39,14 @@ | |||
| private final Sort withinGroupSort; | |||
| private final int maxDocsPerGroup; | |||
|
|
|||
| protected TopGroupsCollector(GroupReducer<T, TopDocsCollector<?>> groupReducer, GroupSelector<T> groupSelector, Collection<SearchGroup<T>> groups, Sort groupSort, Sort withinGroupSort, | |||
| int maxDocsPerGroup, boolean getScores, boolean getMaxScores, boolean fillSortFields) { | |||
| super(groupSelector, groups, groupReducer); | |||
There was a problem hiding this comment.
You can remove a few of these parameters now, I think? They're for the Reducer, and as such don't need to be passed separately.
| topsGroupsActionBuilder.addCommandField(new SearchGroupsFieldCommand.Builder() | ||
| .setField(schema.getField(field)) | ||
| .setGroupSort(groupingSpec.getGroupSort()) | ||
| .setTopNGroups(cmd.getOffset() + cmd.getLen()) |
There was a problem hiding this comment.
Is it reRanking with respect to the "start" param in the solr query?
Looks like the original LTR plugin is supposed to reRank top K irrespective of the offset.
ref.http://lucene.472066.n3.nabble.com/Learning-to-Rank-LTR-with-grouping-td4366691i20.html#a4387929
There was a problem hiding this comment.
True, this currently reranks the top start + rerankDocuments , I'll fix it. Thank you
| @@ -1270,7 +1270,7 @@ private void doProcessGroupedDistributedSearchFirstPhase(ResponseBuilder rb, Que | |||
| final int topNGroups; | |||
| Query query = cmd.getQuery(); | |||
| if (query instanceof AbstractReRankQuery){ | |||
| topNGroups = cmd.getOffset() + ((AbstractReRankQuery)query).getReRankDocs(); | |||
| topNGroups = Math.max(((AbstractReRankQuery)query).getReRankDocs(), cmd.getOffset() + cmd.getLen()); | |||
There was a problem hiding this comment.
Should this be handled for non distributed case as well in doProcessGroupedSearch(..), when pageSize>reRankDocs it returns only "reRankDocs" groups in non distributed case.
There was a problem hiding this comment.
the non distributed case looks fine to me, I'll double check tomorrow
There was a problem hiding this comment.
You were right, I added a unit test and it is failing :/ I'm working on it
| @@ -52,6 +52,7 @@ | |||
| public SecondPassGroupingCollector(GroupSelector<T> groupSelector, Collection<SearchGroup<T>> groups, GroupReducer<T, ?> reducer) { | |||
|
|
|||
| //System.out.println("SP init"); | |||
| //Do we want to check if groups is null here? instead of checking at line 62? | |||
diegoceccarelli
force-pushed
the
master-solr-8776
branch
2 times, most recently
from
013f4da to
c7281e6
Compare
diegoceccarelli
force-pushed
the
master-solr-8776
branch
from
157b946 to
e69a819
Compare
|
updated to the current master |
…ess' code formatting (apache#162)
Update SOLR-8776 to current master